Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python] add method to set/get default configuration #5315

Merged
merged 5 commits into from
Feb 23, 2020

Conversation

tomplus
Copy link
Member

@tomplus tomplus commented Feb 14, 2020

As a follow up the discussion #4485 (comment) I'd like to recovery an option to set default configuration. I prepared very simple solution which provides the same API as we had before.

Thanks for reviewing it.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @Jyhess (2019/01) @slash-arun (2019/11) @spacether (2019/11)

@auto-labeler
Copy link

auto-labeler bot commented Feb 14, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

:return: The configuration object.
"""
if cls._default is not None:
return copy.copy(cls._default)
Copy link
Contributor

@spacether spacether Feb 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why copy it vs just returning the instance?
The CI tests are failing when trying to deepcopy the logger.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning a new Configuration instance which uses the same instantiation inputs as the default instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, deepcopy doesn't work. Here as in the previous implementation a copy of default configuration is returned.

The CI tests failed because a remote endpoint returned 404 error on add_pet method. I don't know why, I'll try to run it again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests passed. I guess the original intention was to return a copy which looks like a new object but with different default values. Maybe I should name the method new_instance instead of get_instance to make it more understandable... ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed method name to new_instance().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new_instance is good.
How do you feel about get_default_copy as a name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, both versions look good to me.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests here look good.
It may also make sense to add a test to make sure that the logger has a different id but I am fine without it.
I have some questions and suggestions about how to implement copy.

@@ -241,6 +244,40 @@ class Configuration(object):
# Disable client side validation
self.client_side_validation = True

@classmethod
def copy(cls, source):
Copy link
Contributor

@spacether spacether Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about changing this to:
__deepcopy__(self, memodict={})
That way you control how everything can be copied. For the logger only you can then make a new logger with the same properties as the old one. All other properties can be deep copied. You can use dir(self) or self.__dict__ to iterate over all of self's properties.

If you do it this way, then in new_instance you can return copy.deepcopy(cls._default)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd totally forgotten about this possibility. Thanks.

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress, please see my other comments

"""
if cls._default is not None:
return copy.deepcopy(cls._default)
return Configuration()
Copy link
Contributor

@spacether spacether Feb 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning None if the default copy is not present?
Then we can do truthy/none checks with the result.
This lets our users see and handle the case when a default is unset.

Or if we want to always return an instance how about naming it
default_copy_or_new_instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think get_default_copy is good enough for this usage.

@@ -63,7 +63,7 @@ class ApiClient(object):
def __init__(self, configuration=None, header_name=None, header_value=None,
cookie=None, pool_threads=1):
if configuration is None:
configuration = Configuration()
configuration = Configuration.get_default_copy()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about?
default_copy = Configuration.get_default_copy()
configuration = Configuration() if default_copy is None else default_copy

Copy link
Contributor

@spacether spacether left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good

@spacether spacether merged commit 3f0c163 into OpenAPITools:master Feb 23, 2020
@tomplus
Copy link
Member Author

tomplus commented Feb 23, 2020

Thank you.

MikailBag pushed a commit to MikailBag/openapi-generator that referenced this pull request Mar 23, 2020
* [python] add method to set/get default configuration

* [python] change method name and fix handling api_key

* python: using modified deepcopy to set/get default configuration

* python: update samples

* python: update samples
@wing328 wing328 added this to the 4.3.0 milestone Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants